feat: GitOps platform-user management (bootstrap superuser SA + reconcile CLI)#1709
feat: GitOps platform-user management (bootstrap superuser SA + reconcile CLI)#1709rohilsurana wants to merge 13 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds audit recording for platform grant and revoke operations in ChangesPlatform audit recording and bootstrap superuser setup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 28393123406Coverage increased (+0.4%) to 44.157%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/serviceuser/service.go (1)
518-531:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
memberrevocation idempotent inUnSudo.
UnSudocurrently returns an error when deleting a non-existentmemberrelation. In remove flows that revoke bothadminandmember, this can fail after successful admin demotion and leave the operation reported as failed.Suggested fix
- if err := s.relationService.Delete(ctx, relation.Relation{ + if err := s.relationService.Delete(ctx, relation.Relation{ Object: relation.Object{ ID: schema.PlatformID, Namespace: schema.PlatformNamespace, }, Subject: relation.Subject{ ID: currentUser.ID, Namespace: schema.ServiceUserPrincipal, }, RelationName: relationName, - }); err != nil { + }); err != nil && !errors.Is(err, relation.ErrNotExist) { return err }core/user/service.go (1)
255-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat missing
memberrelation as no-op duringUnSudo.When
relationService.Deletereturnsrelation.ErrNotExistformember,UnSudofails even though the desired end state is already satisfied. This can break platform-user removal flows that revoke both relations.Suggested fix
- if err := s.relationService.Delete(ctx, relation.Relation{ + if err := s.relationService.Delete(ctx, relation.Relation{ Object: relation.Object{ ID: schema.PlatformID, Namespace: schema.PlatformNamespace, }, Subject: relation.Subject{ ID: currentUser.ID, Namespace: schema.UserPrincipal, }, RelationName: relationName, - }); err != nil { + }); err != nil && !errors.Is(err, relation.ErrNotExist) { return err }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1ae0dde-555e-4c9a-b963-1eee98883348
📒 Files selected for processing (16)
cmd/serve.goconfig/sample.config.yamlcore/serviceuser/mocks/audit_record_repository.gocore/serviceuser/service.gocore/serviceuser/service_test.gocore/user/mocks/audit_record_repository.gocore/user/service.gocore/user/service_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/service_user_service.gointernal/api/v1beta1connect/mocks/user_service.gointernal/api/v1beta1connect/platform.gointernal/api/v1beta1connect/platform_test.gointernal/bootstrap/service.gointernal/bootstrap/service_test.gopkg/auditrecord/consts.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/user/service.go (1)
185-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSwitch
Sudoidempotency to relation-level checks.Line 186-Line 199 uses permission checks (
PlatformCheckPermission) formember. That can short-circuitSudo(..., member)for already-admin users without creating thememberrelation tuple, whileUnSudonow operates on exact relation tuples. This asymmetry can break downgrade flows that expect tuple-level state transitions.Proposed fix
- // check if already su - permissionName := "" - switch relationName { - case schema.MemberRelationName: - permissionName = schema.PlatformCheckPermission - case schema.AdminRelationName: - permissionName = schema.PlatformSudoPermission - } - if permissionName == "" { + // validate requested platform relation + switch relationName { + case schema.MemberRelationName, schema.AdminRelationName: + default: return fmt.Errorf("invalid relation name, possible options are: %s, %s", schema.MemberRelationName, schema.AdminRelationName) } - if ok, err := s.IsSudo(ctx, currentUser.ID, permissionName); err != nil { + // check if the exact relation already exists + if ok, err := s.IsSudo(ctx, currentUser.ID, relationName); err != nil { return err } else if ok { return nil }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6af69031-afee-4758-b2f3-fef5aec7469f
📒 Files selected for processing (5)
core/serviceuser/service.gocore/serviceuser/service_test.gocore/user/service.gocore/user/service_test.gopkg/auditrecord/consts.go
🚧 Files skipped from review as they are similar to previous changes (3)
- core/serviceuser/service_test.go
- core/serviceuser/service.go
- core/user/service_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/user/service_test.go (1)
703-704: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStrengthen audit matcher assertions to lock the full payload contract.
These matchers only validate
EventandTarget.ID. Please also assertResource(ID/Type),Target.Type, andMetadata["relation"]so payload regressions are caught.Proposed matcher shape
- return r.Event == pkgAuditRecord.PlatformAdminAddedEvent && r.Target != nil && r.Target.ID == "test-id" + return r.Event == pkgAuditRecord.PlatformAdminAddedEvent && + r.Target != nil && + r.Target.ID == "test-id" && + r.Target.Type == pkgAuditRecord.UserType && + r.Resource.ID == schema.PlatformID && + r.Resource.Type == pkgAuditRecord.PlatformType && + r.Metadata["relation"] == schema.AdminRelationNameAlso applies to: 817-818, 903-904, 958-959
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc102442-61a1-47be-b539-28d52a165871
📒 Files selected for processing (7)
core/serviceuser/service.gocore/serviceuser/service_test.gocore/user/service.gocore/user/service_test.gointernal/bootstrap/service.gointernal/bootstrap/service_test.gopkg/auditrecord/consts.go
🚧 Files skipped from review as they are similar to previous changes (5)
- core/serviceuser/service_test.go
- core/user/service.go
- internal/bootstrap/service_test.go
- internal/bootstrap/service.go
- core/serviceuser/service.go
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0783d969-56a8-45e7-8f00-957f3e88825f
📒 Files selected for processing (5)
cmd/serve.goconfig/sample.config.yamlinternal/bootstrap/bootstrapuser.gointernal/bootstrap/bootstrapuser_test.gointernal/bootstrap/service.go
| creds.On("Create", mock.Anything, mock.MatchedBy(func(c serviceuser.Credential) bool { | ||
| return c.ID == clientID && c.ServiceUserID == "su-id" && | ||
| c.Type == serviceuser.ClientSecretCredentialType && | ||
| bcrypt.CompareHashAndPassword([]byte(c.SecretHash), []byte("s3cret")) == nil | ||
| })).Return(serviceuser.Credential{ID: clientID}, nil) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files internal/bootstrap/bootstrapuser_test.go
wc -l internal/bootstrap/bootstrapuser_test.go
sed -n '1,220p' internal/bootstrap/bootstrapuser_test.goRepository: raystack/frontier
Length of output: 6008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline internal/bootstrap/bootstrapuser.go --view expanded
sed -n '1,220p' internal/bootstrap/bootstrapuser.go
python3 - <<'PY'
from pathlib import Path
text = Path('internal/bootstrap/bootstrapuser_test.go').read_text()
for needle in ['MatchedBy(func(c serviceuser.Credential)', 'CompareHashAndPassword']:
print(f"{needle}: {text.count(needle)}")
PYRepository: raystack/frontier
Length of output: 7763
Avoid bcrypt inside mock.MatchedBy here. Both Create matchers in this test recompute and verify bcrypt hashes during argument matching, which is expensive enough to stall CI. Capture the serviceuser.Credential passed to creds.Create and assert the hash once after the call instead.
Source: Pipeline failures
| cred, err := creds.Get(ctx, clientID) | ||
| switch { | ||
| case errors.Is(err, serviceuser.ErrCredNotExist): | ||
| return createBootstrapSuperUser(ctx, logger, cfg, clientID, users, creds, promoter) | ||
| case err != nil: | ||
| return fmt.Errorf("bootstrap superuser: get credential %q: %w", clientID, err) | ||
| } | ||
|
|
||
| if bcrypt.CompareHashAndPassword([]byte(cred.SecretHash), []byte(cfg.ClientSecret)) != nil { | ||
| if err := rotateBootstrapSecret(ctx, cfg, clientID, cred, creds); err != nil { | ||
| return err | ||
| } | ||
| logger.InfoContext(ctx, "rotated bootstrap superuser secret", "client_id", clientID) | ||
| } | ||
| return promoteBootstrapSuperUser(ctx, promoter, cred.ServiceUserID) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Fail closed on client_id collisions.
Any existing credential with this client_id is treated as the bootstrap account. If the ID collides with a pre-existing service-user credential, boot will rotate that credential and promote its ServiceUserID to platform admin instead of failing safe. Persist and verify a dedicated bootstrap marker before reusing a credential, or abort startup when the client_id is already owned by something else.
| su, err := users.Create(ctx, serviceuser.ServiceUser{ | ||
| OrgID: schema.PlatformOrgID.String(), | ||
| Title: cfg.title(), | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("bootstrap superuser: create service user: %w", err) | ||
| } | ||
| hash, err := bcrypt.GenerateFromPassword([]byte(cfg.ClientSecret), bootstrapBcryptCost) | ||
| if err != nil { | ||
| return fmt.Errorf("bootstrap superuser: hash secret: %w", err) | ||
| } | ||
| if _, err := creds.Create(ctx, serviceuser.Credential{ | ||
| ID: clientID, | ||
| ServiceUserID: su.ID, | ||
| Type: serviceuser.ClientSecretCredentialType, | ||
| SecretHash: string(hash), | ||
| Title: cfg.title(), | ||
| }); err != nil { | ||
| return fmt.Errorf("bootstrap superuser: create credential: %w", err) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Credential create failures leak orphan bootstrap users.
users.Create happens before the idempotency key exists. If creds.Create fails, startup aborts and the next boot will create another service user because Get(client_id) still returns not found. This needs one transaction or a compensating delete so retries do not accumulate orphan rows.
rohilsurana
changed the title
2 participants
What
Moves platform-superuser management to a GitOps model. Config no longer seeds human
superusers; instead it seeds a single bootstrap superuser service account, and all
ongoing platform-user management (humans and service accounts, admin and member) happens
out-of-band via a declarative
frontier reconcileCLI driven from the IAM repo.Changes
Remove the config human-superuser flow
app.admin.users+MakeSuperUsers+ the boot-timeauthoritativereconciliationthat an earlier revision of this PR had added. No human superusers are seeded from config.
Config-bootstrapped superuser service account (
app.admin.bootstrap.{client_id, client_secret})ClientSecretservice-account credential exists in theplatform org and is promoted to superuser; rotates the secret if it changed. This is the only
config-seeded superuser and the break-glass identity.
client_idmust be a UUID (it is thecredential id); validated with a fail-fast error.
Fix platform-user demotion + relation-selective removal (closes #1710)
UnSudo(ctx, id, relationName)is parameterized and actually strips the requested relation(previously it could only remove
member, neveradmin).RemovePlatformUserhonors the new optionalrelationfield (feat(frontier): add optional relation field to RemovePlatformUserRequest proton#489): when set,removes just that relation (e.g. demote admin → member); empty keeps the remove-both default.
Audit platform grants/revokes
platform.{admin,member}_{added,removed}events via theauditrecordsystem, on both theRPC path (real principal) and boot/config path (system actor,
uuid.Nil). Change-only.Declarative reconcile framework + CLI
internal/reconcile: aReconcilerinterface + registry + kind-based (kind:) multi-doc YAML,with PlatformUser as the first kind (parse → ListPlatformUsers → diff
(principal, relation)→ apply). Future kinds (roles, plans, traits…) plug in without changing the command/format.
frontier reconcile -f <file> [--dry-run] -H <auth>— authoritative converge of the desiredstate; auth via the existing
--headermechanism.Supporting fix
serviceuser_credentialsorg_name made nullable-tolerant (sql.NullString) — a platform-levelservice account has no real organization row.
Testing
internal/reconcile, bootstrap SA, audit,relation-selective removal).
superuser the production way (authenticates as the bootstrap SA and grants the test admin via
AddPlatformUser). The e2e caught two real integration bugs the unit mocks couldn't (UUIDclient_id, nullableorg_name), both fixed here.go build ./...,go vet,gofmt,golangci-lintall clean.Related
RemovePlatformUserRequest.relationfield (merged; this PRbumps the proton pin and regenerates).
Notes
app.admin.bootstrapconfigured, boot is a no-op (no superusers seeded);existing platform relations are untouched.
app.admin.users: that config is removed — seed the bootstrapSA instead and manage humans via GitOps.